fix(cli): align commands/ error messages with 4-ingredient strategy#1255
Open
John-David Dalton (jdalton) wants to merge 8 commits intomainfrom
Open
fix(cli): align commands/ error messages with 4-ingredient strategy#1255John-David Dalton (jdalton) wants to merge 8 commits intomainfrom
John-David Dalton (jdalton) wants to merge 8 commits intomainfrom
Conversation
Rewrites error messages across packages/cli/src/commands/ to follow the What / Where / Saw vs. wanted / Fix strategy from CLAUDE.md's new Error Messages section. Sources: - scan/cmd-scan-create.mts: 5 throws (ecosystems + 4 numeric flags) - scan/cmd-scan-reach.mts: 4 throws (ecosystems + 3 numeric flags) - scan/cmd-scan-list.mts: 2 throws (--page, --per-page) - organization/cmd-organization-dependencies.mts: 2 throws (--limit, --offset) - audit-log/cmd-audit-log.mts: 2 throws (--page, --per-page) - threat-feed/cmd-threat-feed.mts: 1 throw (--per-page) - fix/cmd-fix.mts: 1 logger.fail (--ecosystems) - ask/cmd-ask.mts: 1 InputError (missing QUERY) - login/cmd-login.mts: 1 InputError (non-interactive TTY) - wrapper/add-socket-wrapper.mts: 1 throw (fs.appendFile failure) - wrapper/postinstall-wrapper.mts: 1 throw (nested wrapper setup) - manifest/convert-gradle-to-maven.mts: 1 throw (spawn returned no output) - fix/coana-fix.mts: 1 throw (coana returned non-array JSON) - config/handle-config-set.mts: 1 throw (missing VALUE) Tests updated to match new substrings (regex patterns for easier future evolution). All throws previously typed as plain Error that represent user input validation have been converted to InputError, following the codebase convention. Follows strategy landed in #1254.
This was referenced Apr 22, 2026
Switch `(e as Error).message` to `e instanceof Error ? e.message : String(e)` so that when a non-Error value is thrown (strings, objects, null) the error message stays informative instead of becoming 'undefined'. Same fix as applied to #1260 (iocraft.mts) after Cursor bugbot flagged the pattern on that PR.
Switch the inline `e instanceof Error ? e.message : String(e)` to getErrorCause(e), matching the fleet helper pattern. Same behavior for Error throws; non-Error throws now produce 'Unknown error' instead of '[object Object]'.
Contributor
Author
|
bugbot run |
Cursor bugbot flagged both add-socket-wrapper.mts and postinstall-wrapper.mts for rendering I/O errors with the "Invalid input" title and "Check command syntax with --help" recovery hint (InputError's display mapping). fs.appendFile failures are permission / disk / path problems — FileSystemError renders them as "File system error" with contextual recovery (check file permissions, disk space, etc.). Pass the file path (where available) and the ErrnoException code through so FileSystemError can surface EACCES/ENOSPC/ENOENT-specific recovery text. Reported on PR #1255.
The previous commit swapped InputError → FileSystemError in postinstall-wrapper.mts but left the test's vi.mock factory still exporting InputError, so the source couldn't resolve its FileSystemError import under vitest: [vitest] No "FileSystemError" export is defined on the ".../src/utils/error/errors.mts" mock Replace the mock's InputError stub with a FileSystemError stub that matches the real class's shape (path, code, recovery) so the source code resolves correctly. The existing regex assertion on the error message is unaffected.
Contributor
Author
|
bugbot run |
Two issues flagged on the fresh review of HEAD:
- FileSystemError duplicates the filepath: the message embeds ${file}
AND the constructor receives `file` as the path argument. Since
display.formatErrorForDisplay appends \`(\${error.path})\` automatically,
this surfaced the path twice. Drop the \${file} interpolation from
the message — the path arg alone is enough.
- Scan command numeric flag errors overstated validation: \`--pull-request\`
claimed "non-negative integer" and \`--reach-concurrency\` claimed
"positive integer," but the checks only rejected NaN. Negatives and
floats passed through silently. Tighten the validation to match what
the error messages promise — Number.isInteger + range check at every
call site (cmd-scan-create.mts for both flags, cmd-scan-reach.mts for
reach-concurrency).
The third flagged item (FileSystemError vs InputError in the wrapper
commands) was cursor re-flagging a finding already addressed in commit
769170f — the current code already uses FileSystemError. No action.
Contributor
Author
|
bugbot run |
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix is ON. A cloud agent has been kicked off to fix the reported issue.
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 2269113. Configure here.
Previous commit (2269113) dropped the \${file} interpolation from FileSystemError's message to avoid display.formatErrorForDisplay double-printing the path (which it appends from error.path). The test regex still matched the pre-change format — /failed to append socket aliases to \/etc\/protected-file/ — and wouldn't have matched the new message. Assert on the new shape instead: - regex matches `failed to append socket aliases (Permission denied)` - toMatchObject pins name='FileSystemError' and path='/etc/protected-file' Drive-by: the existing regex had \./etc\/protected-file/ but the message never contained that substring after the FileSystemError swap — so the assertion was silently wrong. Flagged by cursor on #1255.
This comment was marked as resolved.
This comment was marked as resolved.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.

Summary
PR 2 of a multi-PR series that rewrites socket-cli error messages to follow the four-ingredient strategy added in #1254 (What / Where / Saw vs. wanted / Fix).
This PR covers
packages/cli/src/commands/. ~20 error messages, 14 source files, 11 test files.What's fixed
Grouped by the kind of error:
Flag validation (numeric) — same treatment across all:
cmd-scan-create.mts—--pull-request,--reach-analysis-memory-limit,--reach-analysis-timeout,--reach-concurrencycmd-scan-reach.mts—--reach-analysis-memory-limit,--reach-analysis-timeout,--reach-concurrencycmd-scan-list.mts—--page,--per-pagecmd-organization-dependencies.mts—--limit,--offsetcmd-audit-log.mts—--page,--per-pagecmd-threat-feed.mts—--per-pageBefore:
Invalid number value for --pull-request: fooAfter:
--pull-request must be a non-negative integer (saw: "foo"); pass a number like --pull-request=42Flag validation (enum) —
--ecosystems/--reach-ecosystems:Before:
Invalid ecosystem: "foo". Valid values are: npm, pypi, ...After:
--reach-ecosystems must be one of: npm, pypi, ... (saw: "foo"); pass a supported ecosystem like --reach-ecosystems=npmMissing positional:
cmd-ask.mts—QUERYargumenthandle-config-set.mts—VALUEargumentInteractive TTY required:
cmd-login.mts— clarified what "non-interactive" means and pointed atSOCKET_CLI_API_TOKENIO / system failures:
add-socket-wrapper.mts—fs.appendFilefailure now names the file and surfaces the underlying errorpostinstall-wrapper.mts— names both rc files that were attemptedInvariant / internal:
convert-gradle-to-maven.mts— "spawn returned no output" with the binary namecoana-fix.mts—find-vulnerabilitiesnon-array JSON names the actual type receivedError type changes
A bunch of these were
throw new Error(...)for what are clearly input-validation failures. Converted tothrow new InputError(...)to match the codebase convention called out in CLAUDE.md.Tests
11 test files updated — switched to regex matching on the new phrasing. Also added a missing
InputErrorexport to one vi.mock() inpostinstall-wrapper.test.mts.All 218 test files / 2812 tests in
test/unit/commands/pass.What I didn't touch (noted for later)
cmd-fix.mtsuseslogger.fail(...) + process.exitCode = 1; returninstead of throwingInputError. CLAUDE.md calls this pattern out as discouraged ("doesn't set exit code properly" is actually fine here sinceprocess.exitCodeworks, but throwing is cleaner). Not in scope for this PR — just message phrasing.oops/cmd-oops.mtsintentionally throws a vague error — that's the command's purpose.Test plan
socket scan create --pull-request foo .prints the new messagesocket scan list --page -1prints the new messagesocket ask(no args) prints the new messageNote
Low Risk
Low risk: changes are limited to user-facing CLI validation/error messages and switching some throws to
InputError/FileSystemError, with minor tightening of numeric integer checks that could reject previously-accepted invalid values.Overview
Improves Socket CLI command error handling to follow a consistent what/where/saw vs wanted/fix format, replacing several generic
Error/logger messages with clearerInputError(andFileSystemErrorfor wrapper setup) across commands likeask,scan,audit-log,threat-feed,organization dependencies, andconfig set.Tightens validation for several numeric and enum flags (e.g. requiring positive/non-negative integers and clearer
--reach-ecosystems/--ecosystemschoices), and updates unit/integration tests to assert on the new phrasing. Also improves a couple of internal/system failure messages (gradlespawn output missing,coanaJSON shape) and adds a small lockfile metadata tweak.Reviewed by Cursor Bugbot for commit 2269113. Configure here.